-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nixos/top-level: improve replaceRuntimeDependencies #257234
Conversation
8845fc2
to
1940199
Compare
1940199
to
476a209
Compare
c05cabd
to
5b650f1
Compare
This is awesome! I don't know whether it is impolite to ping people, but maybe @ncfavier would be interested in taking a look? |
Can't really review now, but tests will be needed, and, more superficially, use |
5b650f1
to
e6bd44f
Compare
Between this stuff relying on IFD and Hydra not supporting IFD, I'm stuck in a bit of a bad place here. A test that can't run on Hydra seems pretty pointless, and any ideas I have come up with for circumventing this problem incur a significant cost to other people or infrastructure. If you or anyone else can come up with even a vague idea of how to reasonably test things, I will be glad to implement it.
I was unaware that this style is prefered, and also unaware of the performance benefit (although I doubt it matters that much between creating hundreds of derivations). Changed it. |
You could write a shell script that calls Nix and performs all the assertions, use that for (further) development, and wrap it into a NixOS tests so that Hydra runs it as well. |
Yeah, that would have been the "significant cost to infrastructure" solution. I'm under the impression that NixOS tests are already on the expensive side, and having to include nixpkgs itself (so that that the configuration with |
e6bd44f
to
41b3087
Compare
Some tests are now implemented (in a separate commit, so that they can be removed easily if they turn out to be too heavy). I also found two bugs (the |
This comment was marked as outdated.
This comment was marked as outdated.
Today I just faced with |
Note that even this improved version requires IFD. Really, that requirement is very unlikely to go away, because in order to know the references, the build result has to be known. (Technically, you could fetch the entire build input closure as a superset of the references (which would be doable in a very hacky way), but that tends to be extremely large.) |
a5f35de
to
56ddab9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alois31, thank you very much for this wonderful work! I would really love this PR to be merged, and I am slowly reviewing the changes here. This is not easy for me as I am not very familiar with nix internals so it may take a long time... But I will try to post some feedback and questions here as I go through the code. Thank you again for this work!
56ddab9
to
87d7fee
Compare
da79a19
to
ff697c6
Compare
ff697c6
to
202b37c
Compare
202b37c
to
c4b32ac
Compare
acf5ae3
to
e6fdfbe
Compare
Will take a look at this when I have time to. |
Rewrite replaceDependency so that it can apply multiple replacements in one go. This includes correctly handling the case where one of the replacements itself needs to have another replacement applied as well. This rewritten function is now aptly called replaceDependencies. For compatibility, replaceDependency is retained as a simple wrapper over replaceDependencies. It will cause a rebuild because the unpatched dependency is now referenced by derivation instead of by storePath, but the functionality is equivalent. Fixes: NixOS#199162
Instead of iterating over all replacements and applying them one by one, use the newly introduced replaceDependencies function to apply them all at once for replaceRuntimeDependencies. The advantages are twofold in case there are multiple replacements: * Performance is significantly improved, because there is only one pass over the closure to be made. * Correctness is improved, because replaceDependencies also replaces dependencies of the replacements themselves if applicable. Fixes: NixOS#4336
Move replaceRuntimeDependencies to the replaceDependencies namespace, where the structure is more consistent with the replaceDependencies function. This makes space for wiring up cutoffPackages as an option too. By default, the system's initrd is excluded. The replacement process does not work properly anyway due to the structure of the initrd (the files being copied into it, and it being compressed). In the worst case (which has been observed to actually occur in practice), a store path makes it into the incompressible parts of the archive, checksums are broken, and the system won't boot.
The tests cannot be directly built by Hydra, because replaceDependencies relies on IFD. Instead, they are put inside a NixOS test where they are built on the guest.
e6fdfbe
to
1d523a1
Compare
This allows both swapping out and reusing the rewrite machinery.
Unlike regular input-addressed or fixed-output derivations, floating and deferred derivations do not have their store path available at evaluation time, so their outPath is a placeholder. The following changes are needed for replaceDependencies to continue working: * Detect the placeholder and retrieve the store path using another IFD hack when collecting the rewrite plan. * Try to obtain the derivation name needed for replaceDirectDependencies from the derivation arguments if a placeholder is detected. * Move the length mismatch detection to build time, since the placeholder has a fixed length which is unrelated to the store path.
To prevent excessive build times when replacement lists are shared between partially overlapping closures, skip the build of unused replacements. Unfortunately, this also means that the replacement won't be applied any more if another replacement that is applied introduces its source. But this is a corner case, and we already show a warning, so make it clearer that handling this situation (should it ever arise) is the responsibility of the user.
1d523a1
to
8bbea7f
Compare
Description of changes
Instead of iterating over all replacements and applying them one by one, use the newly introduced replaceDependencies function to apply them all at once for replaceRuntimeDependencies. The advantages are twofold in case there are multiple replacements:
Further, the option is moved to a
system.replaceDependencies
namespace more consistent with the arguments to thereplaceDependencies
function. This allows wiring up thecutoffPredicate
functionality, which is also configured to skip the initrd by default because of uselessness in the best case and breakage in the worst case.Fixes: #4336
Fixes: #199162
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)